Spree::OptionValue#name delegates to Spree::OptionType even when nil #3517
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ame to it Spree::OptionValue#name is delegated to Spree::OptionType with option_type prefix (option_type_name). https://github.com/solidusio/solidus/blob/v2.9.5/core/app/models/spree/option_value.rb#L17 The json response of Spree::Api::OptionValuesController uses the *option_value_attributes helper variable which has :option_type_name. If for some reason an option value is created without option_type the above controller will return a 500 error because the name delegation. This commit also remove check_option_values from the option_values_controller_spec.rb because is not used anywhere in the file.
The above line already configures all the routes for the option value resource.
spaghetticode
approved these changes
Feb 17, 2020
There was a problem hiding this comment.
@SamuelMartini thanks for catching this!
I am in favor of this change, I just wonder if we should allow creating option values without option type 🤔
kennyadsl
approved these changes
Feb 18, 2020
There was a problem hiding this comment.
@SamuelMartini thanks. I'm ok with the change and I also can't see a scenario where someone could need an option value without an option type but this is reflected in model validations and it seems to be legit.
kennyadsl
changed the title
kennyadsl
deleted the
SamuelMartini/allow_nil_option_value_method_delegation
branch
waiting-for-dev
added a commit
to nebulab/solidus
that referenced
this pull request
May 26, 2022
It makes no sense to have dangling option_values without an associated option_type. Ref solidusio#3309 & solidusio#3517
|
@SamuelMartini I know this is quite old, but any chance you remember why you needed to create an option value without an option type? |
cpfergus1
pushed a commit
to cpfergus1/solidus
that referenced
this pull request
Aug 25, 2022
It makes no sense to have dangling option_values without an associated option_type. Ref solidusio#3309 & solidusio#3517
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Spree::OptionValue#name is delegated to
Spree::OptionTypewithoption_typeprefix (option_type_name).The json response of
Spree::Api::OptionValuesControlleruses the*option_value_attributeshelper variable which includes :option_type_name.If for some reason an option value is created without option_type the above controller will return a 500 error because of the name delegation.
For instance,
Spree::Api::OptionValuesController#createallows to create an option value record with onlynameandpresentationparams. Once created, the json response view will call*option_value_attributeswithoption_type_namebreaking the app.This pr also:
check_option_valuesfrom theoption_values_controller_spec.rbbecause is not used anywhere in the file.
Checklist: